Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[MXNET-1232] fix demo and add Eclipse support #13979

Merged
merged 5 commits into from
Jan 28, 2019

Conversation

lanking520
Copy link
Member

Description

Get rid of makefile and add Eclipse support for Java
@andrewfayres @piyushghai @zachgk @aaronmarkham

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

</os>
</activation>
<properties>
<mxnet.profile>linux-x86_64-cpu</mxnet.profile>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will always pick the cpu artifact. The existing code allows one to pick up the gpu artifacts as well.

https://github.com/apache/incubator-mxnet/blob/master/scala-package/mxnet-demo/scala-demo/Makefile#L35

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to simplify the process, I decided to get rid of the makefile entirely. To address your concern, I add a noteline in the README to inform then using GPU to run

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. removing makefile was a good riddance :)

I'm fine with adding it to the README.

Copy link
Contributor

@piyushghai piyushghai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Thanks for your contributions! :)

@lanking520
Copy link
Member Author

@aaronmarkham Could you please review this?

Copy link
Contributor

@aaronmarkham aaronmarkham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed many references to "simply" since quite often there are issues and nothing is entirely simple. The implication to someone that fails on a "simple" command is that they have substandard intelligence and cannot complete even the most simple task. 🤷‍♀️ It's just nicer if we avoid assigning complexity levels to various steps.
There are also a lot of "required" statements in here and I think that might not be true. Maven is an option and possibly the easiest, but certainly not the only way to do it.

docs/install/java_setup.md Outdated Show resolved Hide resolved
### Eclipse IDE Support
You can easily convert you existing maven project to a project that can run in Eclipse by:
```
mvn eclipse:eclipse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what directory? Is this known or should you mention it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is known, mvn command could only execute in a folder that has a pom file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also add one line to explain that

docs/install/java_setup.md Outdated Show resolved Hide resolved
scala-package/mxnet-demo/java-demo/README.md Outdated Show resolved Hide resolved
scala-package/mxnet-demo/java-demo/README.md Outdated Show resolved Hide resolved
scala-package/mxnet-demo/java-demo/README.md Outdated Show resolved Hide resolved
## Run in Eclipse
You can convert the maven project to the eclipse one by running
```
mvn eclipse:eclipse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a repeat of before... why is this here?
Also this is a conversion instruction not a "run" instruction as the title suggests. Maybe you can include some screenshots of how you open the project in eclipse and build it, etc...

Copy link
Member Author

@lanking520 lanking520 Jan 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is specifically for users trying to use Eclipse in their demo project
The latter step will just be click Eclipse and click import...

scala-package/mxnet-demo/java-demo/README.md Outdated Show resolved Hide resolved
scala-package/mxnet-demo/scala-demo/README.md Outdated Show resolved Hide resolved
scala-package/mxnet-demo/scala-demo/README.md Outdated Show resolved Hide resolved
@lanking520
Copy link
Member Author

@aaronmarkham please give a 2nd phase review

@sandeep-krishnamurthy sandeep-krishnamurthy added Scala Java Label to identify Java API component pr-awaiting-review PR is waiting for code review labels Jan 25, 2019
Copy link
Contributor

@aaronmarkham aaronmarkham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@lanking520 lanking520 merged commit c4080de into apache:master Jan 28, 2019
jlcontreras pushed a commit to jlcontreras/incubator-mxnet that referenced this pull request Feb 14, 2019
* fix demo and add Eclipse support

* fix on docs

* fix typo

* Update docs/install/java_setup.md

Co-Authored-By: lanking520 <[email protected]>

* add fixes in docs
stephenrawls pushed a commit to stephenrawls/incubator-mxnet that referenced this pull request Feb 16, 2019
* fix demo and add Eclipse support

* fix on docs

* fix typo

* Update docs/install/java_setup.md

Co-Authored-By: lanking520 <[email protected]>

* add fixes in docs
@lanking520 lanking520 deleted the demofix branch March 11, 2019 22:28
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* fix demo and add Eclipse support

* fix on docs

* fix typo

* Update docs/install/java_setup.md

Co-Authored-By: lanking520 <[email protected]>

* add fixes in docs
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Java Label to identify Java API component pr-awaiting-review PR is waiting for code review Scala
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants